-
Notifications
You must be signed in to change notification settings - Fork 2.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add runtime properties to Quarkus builder #43160
Conversation
@@ -54,7 +56,7 @@ public static Map<String, Object> postBuild(Path appClasses, Path pomFile, List< | |||
if (equals == -1) { | |||
throw new RuntimeException("invalid config " + comment); | |||
} | |||
System.setProperty(conf.substring(0, equals), conf.substring(equals + 1)); | |||
runtimeProperties.setProperty(conf.substring(0, equals), conf.substring(equals + 1)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this the real solution?
jbang just always passed Q:CONFIG values in which could both be a mix of build and runtime properties....should we just pass same set of properties as runtime and build properties then ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is the clean way to do it. It didn't work for before because we do not record properties coming from the system. We did this for the quarkus
namespace, but we also removed that with #39604, which explains why it worked in previous versions but not in all cases.
Since system are not recorded, and that is how properties were propagated in the JBang integrations, it caused the issue. To fix it, we needed a way to propagate the properties. I thought about generating a source or a file, but ultimately, I think the API in the builder will work better because we have this needed elsewhere; see #27996.
These runtimeProperties
are also part of the build config (the same as application.properties
). It just tells us that they can be recorded. Regardless, I can also pass these to the build properties to make it more explicit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha. Sounds good and seems @gsmet tested so should be good.
I'll run it through jbang tests when have cr release and should spot if at least keep past fixes :)
030e660
to
be4817d
Compare
Status for workflow
|
I tested it and it looks good for runtime properties. As for build time properties, I tested with |
This allows integrators to set runtime properties to be recorded in the Quarkus build.